[GH-12] Added Ability to re-queue agenda Items. #89

sanjaydemansol · 2021-09-17T05:31:14Z

Fixes #12

Summary

Now old agenda items could be re-queued to upcoming meetings.

Testing notes

Create multiple agenda items for past date according to channel Hash Format.(You may create a message and edit its date manually)
open RHS menu on any of these and click Re-queue

Result:
It will be requeued to upcoming meeting date.

Ticket Link

#12

codecov · 2021-09-17T05:33:35Z

Codecov Report

Base: 24.56% // Head: 22.31% // Decreases project coverage by -2.26% ⚠️

Coverage data is based on head (f4c0c33) compared to base (bc0e65d).
Patch coverage: 11.62% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   24.56%   22.31%   -2.26%     
==========================================
  Files           7        7              
  Lines         403      484      +81     
==========================================
+ Hits           99      108       +9     
- Misses        287      358      +71     
- Partials       17       18       +1

Impacted Files	Coverage Δ
server/command.go	`0.00% <0.00%> (ø)`
server/meeting.go	`39.28% <28.57%> (-0.72%)`	⬇️
server/utils.go	`92.15% <80.00%> (-2.97%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

maisnamrajusingh

LGTM. @mickmister The PR has now been seperated from the ones related to #66

dipak-demansol · 2021-10-13T10:16:50Z

i will check this and update you after some time. @hanzei

mickmister

Great work @sanjaydemansol! This is a really valuable feature, it's great to see it completed.

I have some requests and questions, including a request to write unit tests

mickmister · 2021-10-20T03:04:15Z

webapp/src/actions/index.js

+export function requeueItem(itemId) {
+    return async (dispatch, getState) => {
+        const command = `/agenda requeue ${itemId}`;


Is itemId a post id? If so, it seems like having this requeue slash command doesn't make much sense. 1/5 we should implement this as an HTTP endpoint instead, using the server-side ServeHTTP hook.

@mickmister we had a discussion for same at #88 (comment)

Right, the slash command makes sense then. Maybe we change this to /agenda requeue post ${postId}

Copied over from #88 (comment):

Made /agenda requeue to support future options like
/agenda requeue all # meeting canceled, I want to requeue all items to next meeting.
/agenda requeue last # requeue last items
/agenda requeue last n # requeue last n items

webapp/src/actions/index.js

webapp/src/index.js

server/meeting_test.go

mickmister · 2021-10-20T03:11:40Z

server/utils.go

+	meetingDay := meetingDays[0]
+	for _, day := range meetingDays {
+		if todayWeekday <= day && day != dayToSkip {
+			meetingDay = day
+			break
+		}
+	}


Can we write unit tests for this function?

mickmister · 2021-10-20T03:26:30Z

server/command.go

+			Message:   fmt.Sprintf("#### %v %v) %v", hashtag, numQueueItems, originalPostMessage),
+		})
+		if appErr != nil {
+			return responsef("Error creating post: %s", appErr.Message)


This error should be logged

mickmister · 2021-10-20T03:28:25Z

server/command.go

+		messageRegexFormat = regexp.MustCompile(fmt.Sprintf(`(?m)^#### #%s(?P<date>.*) [0-9]+\) (?P<message>.*)?$`, prefix))
+	)
+
+	if matchGroups := messageRegexFormat.FindStringSubmatch(postToBeReQueued.Message); len(matchGroups) == 3 {


In order to avoid this large indented block here

Suggested change

if matchGroups := messageRegexFormat.FindStringSubmatch(postToBeReQueued.Message); len(matchGroups) == 3 {

matchGroups := messageRegexFormat.FindStringSubmatch(postToBeReQueued.Message)

if len(matchGroups) != 3 {

Then return an error in that if block, and continue on otherwise

great, done.

Seems that this is not done

mickmister · 2021-10-20T03:29:09Z

server/command.go

+		originalPostMessage := strings.TrimSpace(matchGroups[2])
+
+		today := time.Now()
+		local, _ := time.LoadLocation("Local")


Is Local what's used elsewhere in this plugin?

Nope, should I remove it?

What is usually used instead, or is the only instance of LoadLocation?

@sanjaydemansol Please see my question above:

What is usually used instead, or is the only instance of LoadLocation?

No localized methods are used anywhere. removing LoadLocation

time.LoadLocation is stored in local and is used here at.
formattedDate, _ := time.ParseInLocation(hashtagDateFormat, originalPostDate, local)

mickmister · 2021-10-20T03:30:27Z

server/command.go

+
+		itemErr, numQueueItems := calculateQueItemNumber(args, p, hashtag)
+		if itemErr != nil {
+			return itemErr


Suggested change

return itemErr

return errors.Wrap(itemErr, "failed to calculate queue item number")

Our plugin doesn't support returning this as HTTP response.

I think the problem is that itemErr seems like it should be an error but it's a *model.CommandResponse. Maybe we can rename the variable commandResponse?

mickmister · 2021-10-20T03:30:57Z

server/command.go

+		}
+		return responsef(fmt.Sprintf("Item has been Re-queued to %v", hashtag))
+	}
+	return responsef("Make sure, message is in required format!")


Under what circumstances would this occur? We definitely don't want to yell at the user 🙂

Replaced ! with . .It occurs if Hashtag is not correctly formatted.

0/5 it should be worded as

Please make sure the agenda hashtag is formatted properly.

mickmister · 2021-10-21T19:26:09Z

webapp/src/index.js

@@ -19,6 +19,10 @@ export default class Plugin {
            (channelId) => {
                store.dispatch(openMeetingSettingsModal(channelId));
            });
+
+        registry.registerPostDropdownMenuAction('Re-queue', (itemId) => {


registerPostDropdownMenuAction supports a third argument, a callback that returns boolean representing if we should show this action in the post menu. We should use this callback as an opportunity to check if the post is an agenda item post. If we determine that it is not an agenda item post, we should return false in the callback.

We possibly want to use an identifier in post.props to signal that it's an agenda post. But that wouldn't work with manually created agenda items.

0/5, for now we could ensure we only show Re-queue option if a post starts with an #. Your thoughts? @mickmister thanks.

0/5, for now we could ensure we only show Re-queue option if a post starts with an #

I think this is a little too broad of a claim. Two reasons come to mind:

Posts can start with # because they are using a header, and not a hash tag. For example, # I'm a header renders as:

I'm a header

Hashtags don't imply that it's an agenda hashtag.

I think we should go with the post.props strategy. If someone wants to use the post menu to re-queue an item they created manually, too bad. cc @hanzei

Another option going along with this same strategy is for the server-side of the plugin to apply the post.props mutation to the manually created agenda items during the MessageWillBePosted and MessageWillBeUpdated hooks.

👍 for using post.props

Thanks @mickmister @hanzei , nice approach.
Should I implement it in this PR or create a separate issue?

1/5 I'm thinking we should add it to this PR. @hanzei thoughts? I would want the change to block the release anyway

@sanjaydemansol Note that the PR should be using post.props to determine if the post is created by the plugin

dipak-demansol · 2021-10-23T17:10:43Z

@sanjaydemansol i found a issue but i was forgot to update here in PR, pls see this video.

vokoscreenNG-2021-10-18_23-35-10.1.mp4

.

sanjaydemansol · 2021-10-25T03:28:29Z

@sanjaydemansol i found a issue but i was forgot to update here in PR, pls see this video.

vokoscreenNG-2021-10-18_23-35-10.1.mp4
.

Possibly its due to #78

- updates based on feedback - bugfix for date allocation

dipak-demansol

as discussed with Ben, we already have ticket for other issue as #78 so this PR is LGTM

mattermod · 2021-11-30T01:00:59Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

hanzei · 2021-12-15T10:29:40Z

@sanjaydemansol Heads up that this branch has conflicts

mattermod · 2021-12-17T17:03:10Z

The file .circleci/config.yml is in the blocklist for external contributors. Hence, these changes are not tested by the CI pipeline active until the build is re-triggered by a core committer or the PR is merged. Please be careful when reviewing it.
/cc @mattermost/core-security @mattermost/core-build-engineers

mickmister · 2021-12-17T17:21:39Z

/update-branch

mattermod · 2021-12-17T17:21:40Z

We don't have permissions to update this PR, please contact the submitter to apply the update.

sanjaydemansol · 2022-02-10T12:19:19Z

Sure @sanjaydemansol. The changes in .circleci/config.yml are unrelated. Would you please revert them?

@hanzei reverted changes.

Merge fixed

hanzei · 2022-02-10T12:23:03Z

@mickmister Please take another look

mickmister

Thanks @sanjaydemansol, I have a few more requests

mickmister · 2021-12-02T04:07:54Z

server/utils_test.go

+				t.Errorf("nextWeekdayDateInWeekSkippingDay() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, &tt.want) {


Does this also work?

Suggested change

if !reflect.DeepEqual(got, &tt.want) {

require.Equal(t, got, &tt.want) {

@mickmister tested code added require.Equal(t, got, &tt.want) {, not working. test cases failed.

mickmister · 2022-02-10T15:05:50Z

server/command.go

+	}
+
+	var (
+		messageRegexFormat = regexp.MustCompile(fmt.Sprintf(`(?m)^#### #%s(?P<date>.*) [0-9]+\) (?P<message>.*)?$`, prefix))


We should use regexp.Compile instead of MustCompile so we can handle the error and avoid a panic. As well as anywhere else we are calling MustCompile inside of a function.

mickmister · 2022-02-10T15:08:24Z

server/utils_test.go

+	var patch, err = mpatch.PatchMethod(time.Now, func() time.Time {
+		return time.Date(2021, 11, 15, 00, 00, 00, 0, time.UTC)
+	})


I was thinking we would pass in the value for now in the function we're testing this with, rather than implicitly patching the value here. @sanjaydemansol What do you think?

We would modify nextWeekdayDateInWeekSkippingDay to accept now as a parameter:

now, _ := time.Date(2021, 11, 15, 00, 00, 00, 0, time.UTC) got, err := nextWeekdayDateInWeekSkippingDay(now, tt.args.meetingDays, tt.args.nextWeek, tt.args.dayToSkip)

We use now for past and future cases hence, we should be fine with patching.

mickmister · 2022-02-10T15:09:57Z

server/command.go

+	postToBeReQueued, er := p.API.GetPost(oldPostID)
+	if er != nil {
+		return responsef("Error fetching post.")
+	}


0/5 We should log this error

Suggested change

postToBeReQueued, er := p.API.GetPost(oldPostID)

if er != nil {

return responsef("Error fetching post.")

}

postToBeReQueued, appErr := p.API.GetPost(oldPostID)

if appErr != nil {

return responsef("Error fetching post.")

}

Added Logs for error.

mickmister · 2022-02-10T15:11:02Z

server/command.go

+		originalPostMessage := strings.TrimSpace(matchGroups[2])
+
+		today := time.Now()
+		local, _ := time.LoadLocation("Local")


@sanjaydemansol Please see my question above:

What is usually used instead, or is the only instance of LoadLocation?

mickmister · 2022-02-10T15:13:18Z

server/command.go

+
+		itemErr, numQueueItems := calculateQueItemNumber(args, p, hashtag)
+		if itemErr != nil {
+			return itemErr


I think the problem is that itemErr seems like it should be an error but it's a *model.CommandResponse. Maybe we can rename the variable commandResponse?

mickmister · 2022-02-10T15:14:31Z

server/command.go

+		messageRegexFormat = regexp.MustCompile(fmt.Sprintf(`(?m)^#### #%s(?P<date>.*) [0-9]+\) (?P<message>.*)?$`, prefix))
+	)
+
+	if matchGroups := messageRegexFormat.FindStringSubmatch(postToBeReQueued.Message); len(matchGroups) == 3 {


Seems that this is not done

mickmister · 2022-02-10T15:17:13Z

webapp/src/actions/index.js

+export function requeueItem(postId) {
+    return async (dispatch, getState) => {
+        const command = `/agenda requeue ${postId}`;


Since we plan to support other commands in the future like this, I propose we be specific here that we are passing a post id #89 (comment)

Suggested change

const command = `/agenda requeue ${postId}`;

const command = `/agenda requeue post ${postId}`;

@mickmister Updated code as per feedback

mickmister · 2022-02-10T15:25:57Z

server/command.go

+	return nil, numQueueItems + 1
+}
+
+func (p *Plugin) executeCommandReQueue(args *model.CommandArgs) *model.CommandResponse {


It doesn't look like this function was refactored, but we agreed it should be refactored and tested in this conversation

sanjaydemansol · 2022-02-14T13:26:52Z

@mickmister Using Compile in place of MustCompile causes linting issues.

mickmister · 2022-02-14T16:45:49Z

@sanjaydemansol Unrelated to the error message you've posted, but we shouldn't be using the appErr variable name for regular errors, as opposed to *model.AppError.

Is it possible for this regex to be defined on the package level, outside of the function? Then we can use MustCompile

sanjaydemansol · 2022-02-15T06:39:09Z

https://github.com/mattermost/mattermost-plugin-agenda/blob/9457541a516a46efab8b9247dd4822b8688ec3ef/server/meeting.go#L14-L18

@mickmister I think regex is already defined on the package level, outside of the function.

mickmister · 2022-02-15T15:12:26Z

@sanjaydemansol We should follow this convention:

If the regex compilation requires a value at runtime, the compilation needs to occur in a function, so we should use Compile to avoid a panic at runtime.

If the regex compilation does not require a value at runtime, the compilation should occur on the package level, and we should use MustCompile.

sanjaydemansol · 2022-02-16T06:31:19Z

@mickmister updated code as per your suggestion. used MustCompile at package level.

mattermod · 2022-03-05T01:01:07Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @aspleenic

mattermod · 2022-04-04T01:01:01Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

mickmister · 2022-04-25T07:37:57Z

webapp/src/index.js

@@ -19,6 +19,10 @@ export default class Plugin {
            (channelId) => {
                store.dispatch(openMeetingSettingsModal(channelId));
            });
+
+        registry.registerPostDropdownMenuAction('Re-queue', (postId) => {
+            store.dispatch(requeueItem(postId));


We need to use post.props to:

declare a given post was made with the agenda plugin

and check here to see if we should render the Re-queue item in the post dropdown menu

Re-queue an item for next meeting

19f30a6

sanjaydemansol requested a review from jfrerich as a code owner September 17, 2021 05:31

sanjaydemansol requested review from sibasankarnayak, maisnamrajusingh and dipak-demansol September 17, 2021 05:34

maisnamrajusingh approved these changes Sep 17, 2021

View reviewed changes

maisnamrajusingh requested a review from mickmister September 17, 2021 05:49

hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Sep 20, 2021

mickmister added this to the v0.3.0 milestone Oct 19, 2021

mickmister requested changes Oct 20, 2021

View reviewed changes

mickmister reviewed Oct 21, 2021

View reviewed changes

sibasankarnayak approved these changes Nov 3, 2021

View reviewed changes

- create test cases for nextWeekdayDateInWeekSkippingDay

51a8315

- updates based on feedback - bugfix for date allocation

dipak-demansol approved these changes Nov 19, 2021

View reviewed changes

dipak-demansol added QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels Nov 19, 2021

mattermod added the Lifecycle/1:stale label Nov 30, 2021

hanzei requested a review from mickmister November 30, 2021 11:19

sanjaydemansol removed the Lifecycle/1:stale label Dec 5, 2021

merge

e4aa842

hanzei removed Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) Lifecycle/1:stale labels Feb 10, 2022

mickmister requested changes Feb 10, 2022

View reviewed changes

jfrerich removed their request for review February 10, 2022 18:19

- feedback mattermost-community#89

9457541

- update regex Compile => linting fix

f4c0c33

hanzei requested a review from mickmister February 22, 2022 14:21

mattermod added the Lifecycle/1:stale label Mar 5, 2022

hanzei removed the QA Review Done PR has been approved by QA label Mar 5, 2022

maisnamrajusingh self-assigned this Mar 24, 2022

maisnamrajusingh added Awaiting Submitter Action Blocked on the author and removed Lifecycle/1:stale labels Mar 24, 2022

mattermod added the Lifecycle/1:stale label Apr 4, 2022

mickmister reviewed Apr 25, 2022

View reviewed changes

hanzei added Lifecycle/3:orphaned and removed 2: Dev Review Requires review by a core committer Awaiting Submitter Action Blocked on the author Lifecycle/1:stale labels Feb 21, 2023

hanzei closed this Feb 21, 2023

hanzei mentioned this pull request Feb 21, 2023

Re-queue an item for next meeting #12

Open

	if matchGroups := messageRegexFormat.FindStringSubmatch(postToBeReQueued.Message); len(matchGroups) == 3 {
	matchGroups := messageRegexFormat.FindStringSubmatch(postToBeReQueued.Message)
	if len(matchGroups) != 3 {

	return itemErr
	return errors.Wrap(itemErr, "failed to calculate queue item number")

	if !reflect.DeepEqual(got, &tt.want) {
	require.Equal(t, got, &tt.want) {

	const command = `/agenda requeue ${postId}`;
	const command = `/agenda requeue post ${postId}`;

[GH-12] Added Ability to re-queue agenda Items. #89

[GH-12] Added Ability to re-queue agenda Items. #89

Conversation

sanjaydemansol commented Sep 17, 2021

Summary

Testing notes

Ticket Link

codecov bot commented Sep 17, 2021 • edited Loading

Codecov Report

maisnamrajusingh left a comment

Choose a reason for hiding this comment

dipak-demansol commented Oct 13, 2021

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

I'm a header

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dipak-demansol commented Oct 23, 2021

sanjaydemansol commented Oct 25, 2021 • edited Loading

dipak-demansol left a comment

Choose a reason for hiding this comment

mattermod commented Nov 30, 2021

hanzei commented Dec 15, 2021

mattermod commented Dec 17, 2021

mickmister commented Dec 17, 2021

mattermod commented Dec 17, 2021

sanjaydemansol commented Feb 10, 2022

hanzei commented Feb 10, 2022

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

sanjaydemansol commented Feb 14, 2022

mickmister commented Feb 14, 2022

sanjaydemansol commented Feb 15, 2022

mickmister commented Feb 15, 2022

sanjaydemansol commented Feb 16, 2022

mattermod commented Mar 5, 2022

codecov bot commented Sep 17, 2021 •

edited

Loading

mickmister Dec 2, 2021 •

edited

Loading

sanjaydemansol commented Oct 25, 2021 •

edited

Loading

mickmister Feb 10, 2022 •

edited

Loading